feat(ftindex): read Java full-text archives via paimon-ftindex-core#563
Conversation
|
I’ve completed the implementation and am currently waiting for the paimon-full-text release. Once it’s available, I’ll submit the PR. I’d really appreciate your review then. |
Thanks for the heads-up! Could you share which part you've been working on? Tracking issue: #567. You're very welcome to review once it's up. This PR (and #568) is only waiting on the paimon-ftindex-core crates.io release — the dependency is pinned to a GitHub git tag for now, and as soon as the crate is published I'll switch that to the crates.io version and mark the PRs ready. You're very welcome to review once they're up. |
|
Thanks for sharing #568. I've opened #571 as a draft so the scope and implementation are visible. My work focuses on the existing non-primary-key/global-index full-text path and DataFusion integration, rather than the primary-key full-text and hybrid search path covered by #568. It migrates the legacy in-repository Tantivy implementation to There is some overlap with #563 in the dependency and reader wiring. I'm happy to rebase on or reuse that foundation to avoid duplication. Please let me know which integration order you would prefer. I'll also review #568 separately. |
Thanks — the scope is clear now, and I think our work is complementary with essentially no functional overlap: you're migrating the append/global-index full-text path (and DataFusion) onto paimon-ftindex-core, and #568 covers the primary-key full-text + hybrid path. The only real intersection is the shared piece: the paimon-ftindex-core dependency + the FullTextArchiveReader wiring (currently in #563). I'd suggest we land that reader foundation first, as its own small PR (#563), and then both of our stacks — your global-index path and my PK path — rebase onto it so the dependency and reader live in one place. That also lets us settle one shared detail: the pinned tag (I'm on v0.1.0-rc4, you're on v0.1.0-rc5) — we should converge on one and switch to the crates.io version before merging, since a git-only dependency blocks publishing. Works for me to have #563 go in first; happy to adjust the reader's API if your global path needs anything from it. |
|
Thanks, that ordering works for me. One API requirement from #571: global-index archives may be stored on remote FileIO and can be large, so the current implementation intentionally avoids reading the entire archive into memory. It adapts range reads from Could #563 make I'll keep #571 as a draft and rebase it onto #563 once we agree on this API and the foundation lands. |
Done — I've updated #563 with a generic |
|
Thanks for the quick update! I tested #571 on top of the latest #563 and integrated |
Allow both whole-file (SliceReader) and streaming/range-read strategies for full-text archive access. Adds from_seek_read constructor; keeps from_input_file as a convenience wrapper for the whole-file case. Addresses coordination with global-index full-text path.
cce3f55 to
dcf88ba
Compare
Switch from the temporary git tag to the published 0.1.0 release now that the crate is on crates.io, so `cargo package`/publishing of paimon works.
dcf88ba to
217b73d
Compare
JingsongLi
left a comment
There was a problem hiding this comment.
Thanks for adding this shared full-text reader foundation. I reviewed the implementation against paimon-ftindex-core 0.1.0, the existing Tantivy path, and the expected consumers in the PK and global-index read paths.
The reader behavior looks correct, and I did not find a functional search or archive-compatibility issue. I also verified the following locally:
- The two new
ftindex::readertests pass. - The minimal
fulltext + storage-memoryfeature build passes. - Clippy with the
fulltextfeature passes with warnings denied. cargo package -p paimonsucceeds.- All GitHub CI checks are green.
However, I recommend addressing the following API issues before approval.
1. Re-export the core I/O types used by the public API
FullTextArchiveReader<R> and from_seek_read expose the paimon_ftindex_core::io::SeekRead trait in Paimon's public API. The concrete reader returned by from_input_file also contains the core's SliceReader type. However, these types are not re-exported from paimon::ftindex.
As a result, an external consumer cannot implement from_seek_read or explicitly name the in-memory reader type while depending only on paimon; it must add its own direct dependency on paimon-ftindex-core. This partially defeats the goal of keeping the shared reader and dependency wiring in one foundation module, and it also couples consumers to the exact core dependency version selected by Paimon.
Could we either:
- re-export
SeekRead,ReadRequest, andSliceReaderfrompaimon::ftindex; or - hide the core types behind Paimon-owned public traits, adapters, or type aliases?
For example, the module could expose something similar to:
pub use paimon_ftindex_core::io::{ReadRequest, SeekRead, SliceReader};
pub use reader::{FullTextArchiveReader, FullTextHits};Please also add a direct unit test for FullTextArchiveReader::from_seek_read. This is the key API required by the remote FileIO integration in #571, while the current tests only exercise from_input_file.
2. Avoid consuming the include bitmap
search_with_include currently takes RoaringTreemap by value:
pub fn search_with_include(
&self,
query_json: &str,
limit: usize,
include_row_ids: RoaringTreemap,
)The method only serializes the bitmap and does not need ownership. Taking it by value forces callers that reuse an allow-list across multiple archive searches to clone potentially large bitmaps. The planned PK consumer already needs to call include.clone() for this reason.
Please consider accepting &RoaringTreemap instead:
pub fn search_with_include(
&self,
query_json: &str,
limit: usize,
include_row_ids: &RoaringTreemap,
)It would also be useful to return an empty FullTextHits immediately when the include bitmap is empty, instead of executing the full query and filtering every matching document out inside the collector.
3. Correct the memory claim in from_input_file
The documentation currently says that the whole-file implementation keeps peak memory at one archive:
a whole-read keeps peak memory at one archive
However, the implementation first receives a Bytes buffer and then creates a second complete buffer with bytes.to_vec():
let bytes = input.read().await?;
Self::from_seek_read(SliceReader::new(bytes.to_vec()))During the conversion, both buffers coexist, so the temporary peak archive memory is approximately twice the archive size. This may matter for large PK full-text archives if the convenience constructor is used there.
Could we either:
- adjust the documentation so it does not claim one-archive peak memory; or
- provide a
Bytes-backedSeekReadimplementation to avoid the extra full-file copy?
The generic from_seek_read path correctly allows large remote archives to avoid whole-file loading, so this is mainly about making the convenience API and its documentation accurate.
4. Update the PR description
The PR description is stale after the latest commit. It still says:
- the dependency is a Git tag such as
v0.1.0-rc4; - the crate is not available on crates.io;
- the git dependency blocks packaging; and
- the PR should remain Draft until the release.
The current code uses the released registry dependency:
paimon-ftindex-core = { version = "0.1.0", optional = true }The PR is also no longer Draft, and cargo package succeeds. Please update the description so that the final PR history accurately reflects the implementation being merged.
Other than these API and documentation points, the implementation and dependency integration look good.
Map FtIndexError and the roaring row-id filter serialization error into UnexpectedError { source: Some(Box::new(e)) } instead of discarding the underlying error, matching the vindex/lumina convention so the engine error chain survives.
Add a regression test asserting an empty or disjoint include-set admits no rows (not all rows), locking in the search_with_include filter semantics.
- Re-export the core I/O types the public API exposes (SeekRead, ReadRequest, SliceReader) plus BytesReader/FullTextArchiveReader/FullTextHits from paimon::ftindex, so consumers can implement SeekRead or name the reader without depending on paimon-ftindex-core directly. Add a direct from_seek_read unit test. - search_with_include now borrows &RoaringTreemap (it only serializes the bitmap) and short-circuits to empty hits when the allow-list is empty, sparing callers a clone and a wasted query. - Add a Bytes-backed BytesReader and use it in from_input_file so a whole-file read is not copied a second time via SliceReader's Vec<u8>; correct the peak-memory doc accordingly.
8b12a6b to
2cc6c68
Compare
|
Thanks @JingsongLi for the thorough review and for verifying the build/package/CI locally. All four points are addressed in 1. Re-export the core I/O types — 2. Avoid consuming the include bitmap — 3. Memory claim in 4. PR description — updated to reflect the crates.io Verification after the change: |
Purpose
First step of mirroring Java Paimon's primary-key full-text search into the Rust read side. Java's full-text index is backed by a shared native Rust core,
paimon-ftindex-core(public repoapache/paimon-full-text, tantivy 0.26.1), which the Java/Python bindings wrap via JNI/ctypes. This PR lets the Rust reader depend on that same core directly (no JNI needed), so it can read the on-disk archive format Java writes.This is a deliberately small, self-contained foundation slice: dependency wiring + a thin reader wrapper + round-trip tests. It de-risks the external dependency (does it fetch and build here?) before any of the primary-key scan/read machinery is built on top.
Brief change log
paimon-ftindex-core = "0.1.0"(crates.io), wired into the existingfulltextcargo feature viadep:syntax.crates/paimon/src/ftindex/withFullTextArchiveReader<R: SeekRead>:from_seek_read(R)— open over anySeekReadimplementation, so large/remote archives can be range-read without whole-file buffering (used by the append/global-index path in feat(fulltext): read global indexes via paimon-ftindex-core #571).from_input_file(&InputFile)— whole-file convenience wrapper; holds the readBytesdirectly via aBytesReader(no second copy), so peak memory stays at roughly one archive.search(query_json, limit)— JSON-DSL query, returnsFullTextHits { row_ids, scores }.search_with_include(query_json, limit, &RoaringTreemap)— restricts results to a live-row allow-list (thewithIncludeRowIdsequivalent); borrows the bitmap so callers reusing an allow-list need not clone it, and an empty allow-list short-circuits to no hits.SeekRead,ReadRequest,SliceReader) plusBytesReader,FullTextArchiveReader,FullTextHitsfrompaimon::ftindex, so consumers can implementSeekReador name the reader while depending only onpaimon.crate::Errorwith the underlying source preserved, no silent fallback).Out of scope (later PRs): shared PK-index-layer generalization,
CoreOptionsfull-text options, PK full-text scan/read, hybrid-on-PK, and migrating the existing append/data-evolution full-text path off rawtantivy 0.22onto the core.Tests
ftindex::reader::tests::test_round_trip_search_returns_expected_rows— build an archive with the core writer, read it back, assert the correct row-ids/scores.ftindex::reader::tests::test_search_with_include_restricts_to_allow_list— all docs match; the roaring include-filter restricts results to the allow-list subset (also validates cross-crateroaring 0.11compatibility).ftindex::reader::tests::test_search_with_include_empty_or_disjoint_returns_no_hits— an empty or disjoint allow-list admits no rows.ftindex::reader::tests::test_from_seek_read_opens_archive_directly— exercises the genericfrom_seek_readconstructor directly (the API the remote-FileIO path in feat(fulltext): read global indexes via paimon-ftindex-core #571 depends on).cargo test -p paimon --features fulltextgreen;cargo clippy -p paimon --lib --tests --features fulltext -- -D warningsclean; default (no-feature) build, minimalfulltext + storage-memorybuild,cargo package -p paimon, andcargo build -p paimon-datafusion(Send boundary) all pass.API and Format
paimon-ftindex-coreis now depended on from crates.io (paimon-ftindex-core = "0.1.0"), so there is no git dependency,cargo package -p paimonsucceeds, and this PR is ready to merge (no longer draft).No public API change to existing paths and no storage-format change. Adds a new feature-gated reader over the existing Java-written full-text archive format (v1), read through the shared
paimon-ftindex-coreengine. Thefulltextfeature transitionally pulls both tantivy 0.22 (legacy append path) and 0.26 (via the core); the duplicate is removed when the append path converges onto the core in a later PR.Documentation
No documentation changes; module-level doc comments describe the new reader.